Skip to content

Fix: initialise dep_gen on the orchestrator, not behind the handshake - #1486

Merged
ChaoWao merged 1 commit into
hw-native-sys:mainfrom
ChaoWao:fix-depgen-init-race
Jul 26, 2026
Merged

Fix: initialise dep_gen on the orchestrator, not behind the handshake#1486
ChaoWao merged 1 commit into
hw-native-sys:mainfrom
ChaoWao:fix-depgen-init-race

Conversation

@ChaoWao

@ChaoWao ChaoWao commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

The bug

The dep_gen DFX collector intermittently emits an empty deps.json:

AssertionError: deps.json missing expected edges: {(0, 4294967297), ...} (got set())

got set() — no edges at all, rather than wrong edges. The records are never
recorded, not buffered-and-unread and not dropped at emit.

dep_gen_aicpu_init() resolves the collector's buffer state and pops its first
buffer, and it ran from the scheduler cold path after the AICore handshake,
next to pmu_aicpu_init(). pmu belongs there — it needs the physical_core_ids_
the handshake produces. dep_gen needs nothing from it: init only reads
dep_gen_data_base, which kernel.cpp publishes before aicpu_execute() even
starts.

The orchestrator, meanwhile, deliberately skips that handshake
(aicpu_executor.cpp):

if (decouple_orch && is_orchestrator) {
    return 0;  // do NOT touch the handshake or hs_arrived_ barrier
}

so it goes straight to building the graph while the schedulers are still
handshaking cores. Every submit_task in that window reaches
dep_gen_aicpu_record_submit() with s_dep_gen_state still null and is dropped.

Instrumented device log, same binary, same card, only the width changed:

block_dim=3   PASS  dep_gen_aicpu_flush: pre_init_misses=0 init_calls=1 state=0x12c0c00e8440
block_dim=24  FAIL  dep_gen_aicpu_flush: pre_init_misses=5 init_calls=0 state=(nil)
                    dep_gen_aicpu_init:  entering        <-- after the flush

It is a race, not a capacity limit

Handshake wall time scales with core count, so the wider the device the more
often the orchestrator wins:

| block_dim | 2 | 3 | 4 | 6 | 8 | 12 | 20 | 24 |
| --- | --- | --- | --- | --- | --- | --- | --- |
| pass rate | 2/2 | 5/5 | 2/2 | 1/2 | 3/4 | 0/5 | 1/5 | 1/5 |

There is no threshold and no 21/22 cliff — I looked for one, since #1477 had
just fixed a 21-cluster bound in CoreTracker, and this is not that. The
intermediate runs are the giveaway: several failed with tasks[] missing expected ids: {0}, i.e. the graph captured except its very first submit. No
sizing or indexing bug loses exactly the first record and keeps the rest.

This already reaches CI on main. The CI runner loses the race even at
block_dim: 3st-onboard-a2a3 on #1478, which does not touch block_dim at
all, fails with exactly this assertion, and main's own history has an
st-onboard-a2a3 failure among recent runs.

Why only dep_gen

pmu / args_dump / l2_swimlane are per-core subsystems whose producers are
downstream of the same handshake that gates their init, so they cannot race it.
scope_stats is orchestrator-side like dep_gen, but resolves its pointers inside
set_platform_scope_stats_base() — before aicpu_execute() — and pops its
buffer lazily. dep_gen was the only orchestrator-side subsystem with an eager
init parked behind the handshake. I ran all five per-feature smokes at full
width: only dep_gen failed.

The fix

Initialise dep_gen on the thread that feeds it, immediately before the existing
set_orch_thread_idx() and before orch_func_ runs. There is then nothing left
to order it against.

Both cold-path call sites go, not just one: the free_queue is SPSC and the
orchestrator must be its only device-side consumer. a5 carried the identical
arrangement and is fixed with it.

The silence

The failure surfaced as an empty file rather than an error because the early
return skipped the accounting one line below it — the line whose comment reads
"Account every attempted record so total == collected + dropped on host". The
host's reconcile_counters() saw 0/0/0, called that consistent, and wrote
{"tasks":[],"tensors":[],"edges":[]}.

A pre-init record cannot be accounted — the counter lives in the BufferState
that is still null — so it now reports itself once instead. Once, because
record_submit is a per-task path and the AICPU log is not free
(.claude/rules/codestyle.md §7).

Verification

Check Result
dep_gen smoke, widths 24/3/24/16/24/12/24/auto 8/8 pass (was ~80% failure at 24)
pytest examples tests/st --platform a2a3sim 52 passed, 0 failed
pytest examples tests/st --platform a5sim 39 passed, 0 failed
pytest tests/ut -m "not requires_hardware" 816 passed
pre-commit (all hooks) passed

Not run on this branch: the full onboard a2a3 suite and the other four DFX
smokes. The dep_gen smoke is the one that reproduces this, and it was run across
eight widths; the rest is unverified here.

Follow-up, not fixed here

host_build_graph calls dep_gen_aicpu_init()
(src/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_cold_path.cpp:899)
but never calls dep_gen_aicpu_set_orch_thread_idx() or ..._flush(), so
s_orch_thread_idx stays -1 and an enqueue_ready would index queues[-1].
Unreachable today — hbg has no dep_gen consumer path — but it is a separate bug
and wants its own change. Filing an issue.

Unblocks #1478 and #1309, both of which are red on this assertion.

dep_gen_aicpu_init() resolves the collector's buffer state and pops its
first buffer, and it ran from the scheduler cold path after the AICore
handshake, next to pmu_aicpu_init(). pmu belongs there — it needs the
physical_core_ids_ the handshake produces. dep_gen needs nothing from
it: init only reads dep_gen_data_base, which kernel.cpp publishes before
aicpu_execute() starts.

The orchestrator, meanwhile, deliberately skips that handshake:

    if (decouple_orch && is_orchestrator) {
        return 0;  // do NOT touch the handshake or hs_arrived_ barrier
    }

so it goes straight to building the graph while the schedulers are still
handshaking cores. Every submit_task it issues in that window reaches
dep_gen_aicpu_record_submit() with s_dep_gen_state still null and is
dropped. Whether any records survive is a race against handshake wall
time, which scales with core count: at three clusters the schedulers
usually win, at a full device they usually do not, and in between runs
capture a partial graph missing only its first tasks.

Initialising dep_gen on the thread that feeds it removes the race —
there is nothing left to order it against. Both cold-path call sites go,
not just one: the free_queue is SPSC and the orchestrator is now its
only device-side consumer.

The failure was silent because the early return skipped the accounting
one line below it, the line whose comment reads "Account every attempted
record so total == collected + dropped on host". The host's
reconcile_counters() saw 0/0/0, called that consistent, and wrote an
empty deps.json instead of refusing. A pre-init record cannot be
accounted — the counter lives in the BufferState that is still null — so
it now reports itself once instead. Once, because record_submit is a
per-task path and the AICPU log is not free.

a5 carried the identical arrangement and is fixed with it.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

DepGen initialization is moved from scheduler cold paths into the orchestrator flow for both runtime variants. The collector now detects pre-initialization submissions and emits a one-time error before dropping them.

Changes

DepGen sequencing and pre-init handling

Layer / File(s) Summary
Orchestrator initialization order
src/a2a3/runtime/tensormap_and_ringbuffer/{aicpu/aicpu_executor.cpp,runtime/scheduler/scheduler_cold_path.cpp}, src/a5/runtime/tensormap_and_ringbuffer/{aicpu/aicpu_executor.cpp,runtime/scheduler/scheduler_cold_path.cpp}
Scheduler post-handshake paths no longer call dep_gen_aicpu_init(). Both orchestrator paths initialize DepGen before setting the orchestrator thread index.
Pre-initialization submission reporting
src/common/platform/shared/aicpu/dep_gen_collector_aicpu.cpp
A one-time reporting flag is reset during initialization and used to report dropped submissions made before DepGen state exists.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SchedulerContext
  participant AicpuExecutor
  participant DepGenCollector
  SchedulerContext-->>AicpuExecutor: Complete post-handshake setup without DepGen initialization
  AicpuExecutor->>DepGenCollector: dep_gen_aicpu_init()
  AicpuExecutor->>DepGenCollector: dep_gen_aicpu_set_orch_thread_idx(thread_idx)
  AicpuExecutor->>DepGenCollector: Submit dependency record
  DepGenCollector-->>AicpuExecutor: Record or one-time pre-init error
Loading

Possibly related PRs

Poem

A bunny hops where DepGen starts,
Before the tasks and threaded parts.
Cold paths wait, the orch hops in,
One warning marks a pre-start sin.
Then records flow, neat and bright—
Once initialized, all is right.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving dep_gen initialization to the orchestrator path before handshake-dependent work.
Description check ✅ Passed The description matches the diff and explains the dep_gen race, the orchestrator-side fix, and the removed cold-path init sites.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp (1)

1225-1225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the obsolete scheduler-side DepGen guards.

DepGen initialization now belongs exclusively to the orchestrator path, so these empty conditionals add dead control flow and leave misleading scheduler comments.

  • src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L1225-L1225: remove the empty guard and update the surrounding initialization comment.
  • src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L984-L984: remove the empty profiling guard.
  • src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L1221-L1221: remove the empty guard and update the surrounding initialization comment.
  • src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L976-L976: remove the empty profiling guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp`
at line 1225, Remove the obsolete scheduler-side DepGen guards: in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1225 and 984, and
src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1221 and 976, delete the empty guards, remove the empty profiling
guards, and update each surrounding initialization comment to reflect that
DepGen initialization is handled exclusively by the orchestrator path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp`:
- Line 1225: Remove the obsolete scheduler-side DepGen guards: in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1225 and 984, and
src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1221 and 976, delete the empty guards, remove the empty profiling
guards, and update each surrounding initialization comment to reflect that
DepGen initialization is handled exclusively by the orchestrator path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 39545b0b-c3f2-4243-b443-d0eea975496c

📥 Commits

Reviewing files that changed from the base of the PR and between bddd321 and 45a3408.

📒 Files selected for processing (5)
  • src/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cpp
  • src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
  • src/a5/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cpp
  • src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
  • src/common/platform/shared/aicpu/dep_gen_collector_aicpu.cpp

@ChaoWao
ChaoWao merged commit e314323 into hw-native-sys:main Jul 26, 2026
14 of 16 checks passed
@ChaoWao
ChaoWao deleted the fix-depgen-init-race branch July 26, 2026 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant